Refactor few process of VirtualMachineManagerImpl and improve logs by GutoVeronezi · Pull Request #4966 · apache/cloudstack

GutoVeronezi · 2021-04-28T13:27:45Z

Description

This PR intends to refactor a few process, like extract repeated code to methods, remove unnecessary logic... Of class VirtualMachineManagerImpl and improve logging to facilitate troubleshooting.

Types of changes

Breaking change (fix or feature that would cause existing functionality to change)
New feature (non-breaking change which adds functionality)
Bug fix (non-breaking change which fixes an issue)
Enhancement (improves an existing feature and functioanlity)
Cleanup (Code refactoring and cleanup, that may add test cases)

Feature/Enhancement Scale or Bug Severity

Feature/Enhancement Scale

Major
Minor

How Has This Been Tested?

It has been tested locally on a test lab.

DaanHoogland

clgtm

blueorangutan · 2021-05-10T10:21:24Z

Packaging result: ✔️ centos7 ✔️ centos8 ✔️ debian. SL-JID 549

yadvr · 2021-05-10T16:58:17Z

-                .append(broadcastUri)
-                .append("]")
-                .toString();
+        return String.format("NicProfile {\"id\": %s, \"vmId\": %s, \"reservationId\": \"%s\", \"iPv4Address\": \"%s\", \"broadcastUri\": \"%s\"}", id, vmId, reservationId, iPv4Address, broadcastUri);


@GutoVeronezi why are you converting them into hand-written json?

yadvr · 2021-05-10T17:01:17Z

@GutoVeronezi you've submitted many PRs which are the same class of PRs around log improvements and refactorings and changing the resource/vo/object toString() that returns hand-written json and add inconsistency across other VOs/objects/... Why is that? Are the json output from log statements consumed somewhere?

Can you club all such PRs into a single PR with separate/individual commits? They make it difficult to review and test individually and take a lot of time and resources from reviewers and BO/lab.

GutoVeronezi · 2021-05-10T18:03:07Z

@GutoVeronezi you've submitted many PRs which are the same class of PRs around log improvements and refactorings and changing the resource/vo/object toString() that returns hand-written json and add inconsistency across other VOs/objects/... Why is that? Are the json output from log statements consumed somewhere?

Can you club all such PRs into a single PR with separate/individual commits? They make it difficult to review and test individually and take a lot of time and resources from reviewers and BO/lab.

@rhtyd

Most of toString() implementations uses a simple hyphen (-) to separete values, but what NicProfile[1-2-3-192.168.0.1-...] means? Operators get confused and need to open the code to know what each value means. Moreover, if a string contains a hyphen, it will print as an separator and can confuse much more the operator.
JSON was chosen because it is an easy format to read key-value pairs. NicProfile {"id": 1, "vmId": 2, "reservationId": "3", "iPv4Address": "192.168.0.1", "broadcastUri": "..."} tells much more than a simple -----.

But that is not the focus of the PR. The focus is improve logging of these classes, as they do not supply an appropriate context to make the operation feasible.

About join all this PRs in only one (separated in commits):
Each PR is separted by context. Each one has their own peculiarity. Each peculiarity refactored is separated by commits. Join this all in a single commit will make it even more difficult to review, as there will be a lot of changes without punctual descriptions.

GabrielBrascher · 2021-05-10T19:36:20Z

Here follow my 2cents.

1 - Number of PRs

@rhtyd raised an interesting point regartding the quantity of PRs.
I agree that there are quite a lot of PRs, and that this can make our life harder to track, test, and review them all. However, I understand that many of them hold their particular contexts.

I think that creating one issue to track these PRs would help on testing and reviewing them all. The idea would be to link any PR that fits this effort on enhancing log messages as well as keeping a track of each PR (merged, under review, draft, etc).

2 - Creating an log message pattern

I have been running 4.16-SNAPSHOT already with a couple of these toString() methods. Personally I am happy to see log messages with a pattern and holding solid information. Maybe we can find better formatations than the JSON like. However, I find them interesting as it is a globally understood pattern for anyone at IT.

The biggest plus in adopting toString is the standardtization on log messages. Nowadays we see all kind of ways of logging resources. Some logs present the name, or the ID, sometimes just the UUID, or combinations of them. It is about time to promote a standard (which would be possible mostly due to toString).

DaanHoogland · 2021-05-11T06:32:11Z

@rhtyd please let's make changes per PR as little as possible. smaller PRs are usually easier to review. Only when the code is very intertwined should we combine PRs into bigger ones.

yadvr · 2021-05-11T08:09:13Z

@DaanHoogland I've just shared my preference that is around optimising around reviewers' and lab resources time. One can get the same thing with a single PR with individual small commits that are easier to review (Github allows you to review each commit individually). The benefit of the latter is you save a lot of overhead in people's time in tracking/test, and on smoketest resources.

@GutoVeronezi what you've raised is a valid issue, however, there was no discussion around a new standard/consistent format for people to follow for new code. For example and as a suggestion, instead of NicProfile {"id": 1, "vmId": 2, "reservationId": "3", "iPv4Address": "192.168.0.1", "broadcastUri": "..."}, something like NicProfile [id=1, vmId=2, reservationId=3, iPv4Address=192.168.0.1, ...] may read better, and developers don't need to work with handwritten json. This is merely an example - I'm not asking you to change the string format.

Here something to think about:

You can present the enhancement cleanups under a standard initiative, start discussion on dev@, i.e. what should be a new standard on improving logs, what new developers, new feature/changes should follow. For example, Gabriel's suggestion on a standard logging pattern is great feedback.
A central utility/method that can help switch between formats of objects (json, csv, ...), if that is even feasible? Do a poc with your proposal.
Should we log internal DB IDs in logs? Is the cleanup PRs is replacing old bad logging style in a new way?
I've seen customers/users who pass logs through an aggregation system such as Splunk where they may have rules to make sense of things. Many old dinosaurs in the community like me are used to a pattern/style of reading logs and code i.e. have we considered what could/would break for existing users and developers, or their approach as their eyes are set looking for something they'll not adjust (written with humour - this point can be completely ignored we don't maintain backward compatibility for logs)

On the issue of context, frankly, any PR that is more than a 100 lines of changes is not going to get a thorough review for each and every line by 2xreviewers, it's simply not feasible at least for me and people I know. Instead, we use smoketests and other means (manual QA, design/architecture review, consistency/pattern of changes, ...) to validate such PRs. I would club all such PRs around the same initiative which in this particular case is around logging/refactoring improvements. Splitting the work around the same initiative in 10 different PRs camouflaging under different contexts will just make the process a lot slower and you're essentially in some way telling the community (of dinosaurs :D) that you want them to work hard in their "free" time and you essentially don't care of the overhead it causes them on reviewing/tracking/testing/...

yadvr · 2021-06-18T01:12:45Z

@blueorangutan package

blueorangutan · 2021-06-18T01:13:02Z

@rhtyd a Jenkins job has been kicked to build packages. I'll keep you posted as I make progress.

blueorangutan · 2021-06-18T02:26:17Z

Packaging result: ✔️ centos7 ✔️ centos8 ✔️ debian. SL-JID 269

blueorangutan · 2021-09-15T19:08:42Z

Trillian test result (tid-2053)
Environment: xenserver-71 (x2), Advanced Networking with Mgmt server 7
Total time taken: 40902 seconds
Marvin logs: https://github.com/blueorangutan/acs-prs/releases/download/trillian/pr4966-t2053-xenserver-71.zip
Smoke tests completed. 89 look OK, 0 have errors
Only failed tests results shown below:

Test	Result	Time (s)	Test File

blueorangutan · 2021-09-15T19:41:21Z

Trillian test result (tid-2055)
Environment: vmware-65u2 (x2), Advanced Networking with Mgmt server 7
Total time taken: 41000 seconds
Marvin logs: https://github.com/blueorangutan/acs-prs/releases/download/trillian/pr4966-t2055-vmware-65u2.zip
Smoke tests completed. 89 look OK, 0 have errors
Only failed tests results shown below:

Test	Result	Time (s)	Test File

blueorangutan · 2021-09-15T19:49:19Z

Trillian test result (tid-2054)
Environment: kvm-centos7 (x2), Advanced Networking with Mgmt server 7
Total time taken: 41239 seconds
Marvin logs: https://github.com/blueorangutan/acs-prs/releases/download/trillian/pr4966-t2054-kvm-centos7.zip
Smoke tests completed. 84 look OK, 5 have errors
Only failed tests results shown below:

Test	Result	Time (s)	Test File
test_01_add_primary_storage_disabled_host	`Error`	0.63	test_primary_storage.py
test_01_primary_storage_nfs	`Error`	0.12	test_primary_storage.py
ContextSuite context=TestStorageTags>:setup	`Error`	0.21	test_primary_storage.py
ContextSuite context=TestKubernetesCluster>:teardown	`Error`	77.00	test_kubernetes_clusters.py
test_02_list_snapshots_with_removed_data_store	`Error`	1.19	test_snapshots.py
test_01_secure_vm_migration	`Error`	178.17	test_vm_life_cycle.py
test_02_unsecure_vm_migration	`Error`	278.34	test_vm_life_cycle.py
test_03_secured_to_nonsecured_vm_migration	`Error`	149.01	test_vm_life_cycle.py
test_08_migrate_vm	`Error`	44.78	test_vm_life_cycle.py
test_hostha_enable_ha_when_host_in_maintenance	`Error`	303.83	test_hostha_kvm.py

nvazquez · 2021-09-16T03:38:13Z

Thanks for reviewing @rhtyd I see your points - opening for discussion @GutoVeronezi @sureshanaparti @rhtyd @GabrielBrascher I think we all agree this PR will need some extensive testing for regressions on VM lifecycle operations. A good sign is that smoke tests are returning good results. Given that we are close to cutting the RC, do you think we can complete all the required testing in time for 4.16?

yadvr · 2021-09-16T05:25:37Z

@nvazquez General comment - closer to an RC you want to exclude any non-essential PRs you aren't confident on wrt stability and potential regressions. We can move such PRs to merging right after cutting the RC, and move them to the next major or minor release which should give enough time for community to test the changes until the next major/minor release.

nvazquez · 2021-09-20T00:34:41Z

@rhtyd makes sense, thanks. If no objections let's move this PR to the next milestone to prevent unexpected regressions close to the RC cut date

DaanHoogland · 2021-10-11T07:16:51Z

@nvazquez @rhtyd does it make sense to move this to the milestone 4.16.1.0? it is not a new feature and was built for 4.16.

GutoVeronezi · 2021-11-11T13:24:36Z

Restart jenkins...

GutoVeronezi · 2021-11-18T16:53:17Z

Restart jenkins...

DaanHoogland · 2021-11-22T09:28:53Z

@blueorangutan package

blueorangutan · 2021-11-22T09:30:06Z

@DaanHoogland a Jenkins job has been kicked to build packages. I'll keep you posted as I make progress.

blueorangutan · 2021-11-22T10:11:38Z

Packaging result: ✔️ el7 ✔️ el8 ✔️ debian ✔️ suse15. SL-JID 1757

DaanHoogland · 2021-11-22T12:23:07Z

@blueorangutan test

blueorangutan · 2021-11-22T12:24:03Z

@DaanHoogland a Trillian-Jenkins test job (centos7 mgmt + kvm-centos7) has been kicked to run smoke tests

blueorangutan · 2021-11-22T21:03:24Z

Trillian test result (tid-2573)
Environment: kvm-centos7 (x2), Advanced Networking with Mgmt server 7
Total time taken: 29783 seconds
Marvin logs: https://github.com/blueorangutan/acs-prs/releases/download/trillian/pr4966-t2573-kvm-centos7.zip
Smoke tests completed. 91 look OK, 0 have errors
Only failed tests results shown below:

Test	Result	Time (s)	Test File

@GabrielBrascher

…pache#4966) * Improve logs * Remove unnecessary comments * Use diamond inference * Fix some logs * Remove unnecessary unboxing * Create method to handle job result * Remove unused vars and fix some logics * Extract code to method and few adjusts * Use CollectionUtils * Extract pending work job validation to method * Create new constructors * Extract work job and info creation to a method * Extract submit async job to a method * Extract find vm by id to a method * Change log level from trace to debug * Remove unnused methods and add logs * Undo code remotion * Remove asserts and fix conditionals * Address @GabrielBrascher reviews * Remove double quotes from keys in manual json * Undo code remotion * Add object to log * Remove statement from try/catch * Implement toString with ReflectionToStringBuilderUtils * Fix errors related to merge main Co-authored-by: Daniel Augusto Veronezi Salvador <daniel@scclouds.com.br>

@GabrielBrascher

…pache#4966) (apache#211) * Improve logs * Remove unnecessary comments * Use diamond inference * Fix some logs * Remove unnecessary unboxing * Create method to handle job result * Remove unused vars and fix some logics * Extract code to method and few adjusts * Use CollectionUtils * Extract pending work job validation to method * Create new constructors * Extract work job and info creation to a method * Extract submit async job to a method * Extract find vm by id to a method * Change log level from trace to debug * Remove unnused methods and add logs * Undo code remotion * Remove asserts and fix conditionals * Address @GabrielBrascher reviews * Remove double quotes from keys in manual json * Undo code remotion * Add object to log * Remove statement from try/catch * Implement toString with ReflectionToStringBuilderUtils * Fix errors related to merge main Co-authored-by: Daniel Augusto Veronezi Salvador <daniel@scclouds.com.br> Co-authored-by: Daniel Augusto Veronezi Salvador <38945620+GutoVeronezi@users.noreply.github.com> Co-authored-by: Daniel Augusto Veronezi Salvador <daniel@scclouds.com.br>

GutoVeronezi marked this pull request as draft April 28, 2021 20:28

GutoVeronezi marked this pull request as ready for review April 28, 2021 22:35

GutoVeronezi marked this pull request as draft April 29, 2021 16:44

GutoVeronezi marked this pull request as ready for review April 29, 2021 20:35

apache deleted a comment from blueorangutan May 3, 2021

DaanHoogland approved these changes May 3, 2021

View reviewed changes

GutoVeronezi force-pushed the improve-logs-on-virtualmachinemanagerimpl branch from 8c69a60 to 10ee79b Compare May 4, 2021 15:41

apache deleted a comment from blueorangutan May 10, 2021

yadvr reviewed May 10, 2021

View reviewed changes

apache deleted a comment from blueorangutan May 25, 2021

GutoVeronezi mentioned this pull request May 25, 2021

Improve logs on RemoteAccessVpnManagerImpl #4960

Merged

7 tasks

GutoVeronezi force-pushed the improve-logs-on-virtualmachinemanagerimpl branch from 10ee79b to d3a80ce Compare May 31, 2021 16:16

nvazquez modified the milestones: 4.16.0.0, 4.17.0.0 Sep 20, 2021

GutoVeronezi added 3 commits September 27, 2021 14:03

Merge branch 'main' into improve-logs-on-virtualmachinemanagerimpl

4524128

Merge branch 'main' into improve-logs-on-virtualmachinemanagerimpl

7ef522b

Merge branch 'main' into improve-logs-on-virtualmachinemanagerimpl

24b30cb

Merge branch 'main' into improve-logs-on-virtualmachinemanagerimpl

57ce0ed

GutoVeronezi closed this Nov 11, 2021

GutoVeronezi reopened this Nov 11, 2021

Merge branch 'main' into improve-logs-on-virtualmachinemanagerimpl

9fcf33d

GutoVeronezi closed this Nov 18, 2021

GutoVeronezi reopened this Nov 18, 2021

Fix errors related to merge main

732bbcd

DaanHoogland merged commit ddd2fca into apache:main Nov 23, 2021

Conversation

GutoVeronezi commented Apr 28, 2021

Description

Types of changes

Feature/Enhancement Scale or Bug Severity

Feature/Enhancement Scale

How Has This Been Tested?

Uh oh!

DaanHoogland left a comment

Choose a reason for hiding this comment

Uh oh!

blueorangutan commented May 10, 2021

Uh oh!

yadvr May 10, 2021

Choose a reason for hiding this comment

Uh oh!

yadvr commented May 10, 2021

Uh oh!

GutoVeronezi commented May 10, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

GabrielBrascher commented May 10, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

1 - Number of PRs

2 - Creating an log message pattern

Uh oh!

DaanHoogland commented May 11, 2021

Uh oh!

yadvr commented May 11, 2021

Uh oh!

yadvr commented Jun 18, 2021

Uh oh!

blueorangutan commented Jun 18, 2021

Uh oh!

blueorangutan commented Jun 18, 2021

Uh oh!

blueorangutan commented Sep 15, 2021

Uh oh!

blueorangutan commented Sep 15, 2021

Uh oh!

blueorangutan commented Sep 15, 2021

Uh oh!

nvazquez commented Sep 16, 2021

Uh oh!

yadvr commented Sep 16, 2021

Uh oh!

nvazquez commented Sep 20, 2021

Uh oh!

DaanHoogland commented Oct 11, 2021

Uh oh!

GutoVeronezi commented Nov 11, 2021

Uh oh!

GutoVeronezi commented Nov 18, 2021

Uh oh!

DaanHoogland commented Nov 22, 2021

Uh oh!

blueorangutan commented Nov 22, 2021

Uh oh!

blueorangutan commented Nov 22, 2021

Uh oh!

DaanHoogland commented Nov 22, 2021

Uh oh!

blueorangutan commented Nov 22, 2021

Uh oh!

blueorangutan commented Nov 22, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

GutoVeronezi commented May 10, 2021 •

edited

Loading

GabrielBrascher commented May 10, 2021 •

edited

Loading